Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xarray version update #323

Merged
merged 14 commits into from
May 12, 2023
Merged

xarray version update #323

merged 14 commits into from
May 12, 2023

Conversation

dabhicusp
Copy link
Collaborator

Solution for the TODO of issue #291 TODO: Find a better way for logging the dataset size. is just updating the version of xarray.

The problem with the current version of xarray which we are using is it takes too much memory while calculating dataset.nbytes (eg: Line #408 weather-mv) and kills the process as well.

To address the problem, I reported the issue on the official xarray GitHub page pydata/xarray#7772 and they suggested to use the latest version of xarray. After testing, I found that xarray v2023.1.0 worked well, and we no longer experienced any memory issues.

ps: This is the last python 3.8 supported version of xarray.

@mahrsee1997 mahrsee1997 requested a review from alxmrs April 27, 2023 14:00
Copy link
Collaborator

@alxmrs alxmrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks great! Thanks for the PR.

weather_mv/loader_pipeline/sinks_test.py Outdated Show resolved Hide resolved
weather_mv/loader_pipeline/sinks_test.py Show resolved Hide resolved
Copy link
Collaborator

@alxmrs alxmrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a concern.

@@ -31,5 +30,6 @@ dependencies:
- pygrib=2.1.4
- pip:
- earthengine-api==0.1.329
- xarray==2023.1.0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second though: we may get weird link errors with our conda deploy if we don't include this as a conda dependency. Has this been published on conda forge yet?

Copy link
Collaborator Author

@dabhicusp dabhicusp May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While trying to install the xarray=2023.1.0 package in the CI/CD pipeline via the conda-forge channel, I encountered an error. However, the installation process worked without any issues when I tried it locally. Here's an error message that I received during the CI/CD run:
image
Link
Do you have any suggestions for resolving this issue?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like 2023.4.2 is available on conda (I think this is the latest release). How about we try upgrading to that version?

https://github.com/conda-forge/xarray-feedstock#current-release-info

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in the PR Description This is the last updated version of xarray that python 3.8 supported.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm getting ahead of myself. I'm happy to keep the pip (vs the conda) version, if we can test our local and docker deployment to prove that there are no link errors. Let's also create a follow-up CL to deprecate python 3.8 and include python 3.10. How does this sound?

Copy link
Collaborator Author

@dabhicusp dabhicusp May 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @alxmrs, I have tested both the local and docker deployments and did not encounter any errors.

Yes, this sounds good to me.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for confirming!

weather_mv/loader_pipeline/sinks_test.py Outdated Show resolved Hide resolved
weather_mv/loader_pipeline/sinks_test.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@alxmrs alxmrs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants